-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: updates for login_app type [LIBS-405] #788
Conversation
cli/src/lib/compiler/compile.js
Outdated
@@ -67,7 +68,7 @@ const compile = async ({ | |||
mode = 'development', | |||
watch = false, | |||
}) => { | |||
const isApp = config.type === 'app' | |||
const isApp = appTypes.includes(config.type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could export a convenience method .. isApp
instead of repeating the .includes()
around
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
if (loginApp) { | ||
const fakeSystemInfo = { version: '2.40-SNAPSHOT' } | ||
setState({ loading: false, systemInfo: fakeSystemInfo }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can return
at the end of the if (loginApp)
branch .. to avoid the else
and nesting for the rest, and keep it similar to other src/index.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
adapter/src/components/AppWrapper.js
Outdated
@@ -34,3 +37,28 @@ AppWrapper.propTypes = { | |||
children: PropTypes.node, | |||
plugin: PropTypes.bool, | |||
} | |||
|
|||
export const LoginAppWrapper = ({ children }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can move LoginAppWrapper into its own directory with supporting hooks, to keep the Login bits contained .. and avoid a lot of branching other than at the top level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put it in a separate file. I wasn't sure about putting it in a different directory as it uses a number of the same components (in the same directory) as AppWrapper
Closed and replaced by #831. This original PR is against beta and has some functionality that is no longer relevant as we have decided against facilitating custom login apps (and hence do not need to build a useLoginConfig hook into app-runtime) |
This PR has changes necessary to build "public" login apps. A login app needs to not include the header bar (😅) but also needs to not rely on any requests to endpoints beyond the auth wall: hence the updates. See ticket https://dhis2.atlassian.net/jira/software/c/projects/LIBS/issues/LIBS-405
Note: regardless of whether we want to facilitate the building of custom login apps, we will need to implement most points of this PR. The last point (passing information about the app being of login_app type to app-runtime) is only necessary for facility hook in app-runtime (for building custom login apps)
changes
Header Bar
removed
UI Language
We will need to be able to update the UI language within our login app, so we could choose to not set the i18n language within app-platform for our login app, but I think it makes sense to do this generally (e.g. for creating general login apps) (
uiLocale
will be available publicly (DHIS2-14681) and the struts login app effectively defaults to this, but we could also use the browser language as the default language)API Version
I think we will want the api version to be accessible to the app (e.g. for feature toggling)? It's also used in the app-runtime/services/data (e.g. https://github.com/dhis2/app-runtime/blob/master/services/data/src/links/RestAPILink.ts#L21). Currently, I don't think we have planned for this being accessible publicly?
Login Modal/Base URL
I'm not sure if we want to provide something similar to the LoginModal so that users can specify a base url? Otherwise, we could just have users
Offline capabilities
As discussed in our FE call (21 Feb 2023), we don't need offline support for
login_app
s, so I've removed the PWALoadingBoundary and OfflineInterfaceProvider from app-adapter, but I'm not sure if it's sufficient to just pass anull
value forofflineInterface
down to app-runtime Provider (https://github.com/dhis2/app-runtime/blob/master/runtime/src/Provider.tsx#L21) or if we need to adapt app-runtime as well?Communicating type to app-runtime
This change is only applicable if we want to facilitate building custom login apps. For more information on facilitating custom login apps, see dhis2/ui#1395.
Whether app is of login_app type is sent to provider/app-runtime (f6abdd0#diff-113d225207bf5868bc80a76be8f1d7bd9eeaa06c184a8605ffb80bb78cf16e64R81) in order to facilitate the useLoginSettings hook (e.g. to populate this hook only for login apps).
For how this would work in app-runtime, see dhis2/app-runtime#1352